Skip to content

dd: page-align the read buffer for iflag=direct#13373

Open
relative23 wants to merge 5 commits into
uutils:mainfrom
relative23:dd-odirect-buffer-alignment
Open

dd: page-align the read buffer for iflag=direct#13373
relative23 wants to merge 5 commits into
uutils:mainfrom
relative23:dd-odirect-buffer-alignment

Conversation

@relative23

@relative23 relative23 commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Fixes #12085.

Problem

dd iflag=direct can fail with EINVAL because Vec<u8> does not guarantee the alignment required by O_DIRECT.

Fix

Use a safe Vec-backed buffer that exposes a page-aligned fixed-size slice. Read helpers return the valid prefix instead of resizing the buffer, and the initial buffer size respects count when less data is requested. This adds no dependency or unsafe code.

Validation

@xtqqczze

This comment was marked as resolved.

@relative23
relative23 force-pushed the dd-odirect-buffer-alignment branch from ff5279f to d57f06a Compare July 12, 2026 22:52
@relative23

Copy link
Copy Markdown
Contributor Author

Updated the description to properly credit #12143, which I had missed when opening this (my issue search didn't match its title). See the "Relation to existing PRs" section.

@codspeed-hq

codspeed-hq Bot commented Jul 13, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 339 untouched benchmarks
⏩ 46 skipped benchmarks1


Comparing relative23:dd-odirect-buffer-alignment (3d7e723) with main (be9fb63)

Open in CodSpeed

Footnotes

  1. 46 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@github-actions

github-actions Bot commented Jul 13, 2026

Copy link
Copy Markdown

GNU testsuite comparison:

Skip an intermittent issue tests/tail/retry (fails in this run but passes in the 'main' branch)
Skipping an intermittent issue tests/date/date-locale-hour (passes in this run but fails in the 'main' branch)
Congrats! The gnu test tests/seq/seq-epipe is now passing!

@relative23

Copy link
Copy Markdown
Contributor Author

Thanks for the report — breakdown of the two kinds of deltas:

Memory (all six: exactly +4.0 KB): this is the intentional over-allocation by one page that provides the alignment guarantee (capacity + page_size, the safe-code equivalent of GNU dd's ptr_align(ibuf, page_size)), so it's a constant +4 KiB regardless of block size, not a leak or scaling issue.

Simulation (−4…−5%): per-iteration bookkeeping of the new buffer wrapper (capacity check + slice re-borrow), which adds up at bs=512/bs=4K where the copy loop runs tens of thousands of iterations. Addressed in 9eecf33 with a steady-state fast path in resize plus inlining. Local divan wall-time medians (same machine, 100 samples), main vs. this branch after the fix:

bench main this PR
dd_copy_default 23.8 ms 23.0 ms
dd_copy_8k_blocks 6.68 ms 6.39 ms
dd_copy_64k_blocks 12.4 ms 12.0 ms
dd_copy_partial 749 µs 730 µs

Worth keeping in mind when weighing the remaining instruction-count delta: these benchmarks don't use direct I/O, and CodSpeed doesn't measure syscall cost — while the bug this PR fixes currently makes every full-block oflag=direct write go through a failed write + two fcntl calls per block (and makes iflag=direct reads fail outright).

(The Android job failure is unrelated — it died in "Create and cache emulator image".)

@relative23

Copy link
Copy Markdown
Contributor Author

Verified the fast path against the gate's own metric (instruction counts, single-pass analysis-mode bench binaries under valgrind --tool=cachegrind, whole-process I refs, main vs. this branch):

bench main this PR delta
dd_copy_default 37,668,379 38,195,426 +1.40%
dd_copy_4k_blocks 4,741,075 4,809,777 +1.45%
dd_copy_partial 3,229,459 3,244,042 +0.45%

Down from the reported 4–5% to well under 1.5% (≈8 instructions per copied block, from the remaining slice re-borrows). The constant +4 KiB in the memory-mode numbers is the page over-allocation that provides the alignment guarantee itself.

@relative23

Copy link
Copy Markdown
Contributor Author

Round 3, following up on the flame graphs from the last run: Input::read and Output::write_blocks were instruction-identical to main, so the residual simulation delta sat in the copy loop itself — every Deref of AlignedBuffer re-materialized the data slice (bounds check + RawVec::ptr reconstruction, ~4% combined on dd_copy_4k_blocks), plus the leftover per-iteration resize/truncate bookkeeping.

Rather than keep shaving at wrapper overhead, b859333 removes it structurally by consolidating with the read path from #12143 (credit to @chrboe — this is essentially his slice-based design on top of the safe runtime-page-size allocation):

  • the copy loop takes the byte slice once, outside the loop; AlignedBuffer shrinks to an allocation helper and its Deref never appears in the hot loop
  • fill_consecutive/fill_blocks operate on &mut [u8] and return the number of valid bytes instead of truncating
  • conv=block/unblock output moves to a separate scratch Vec (those conversions can change the byte count); everything else stays in place
  • the per-iteration resize is gone entirely, which also retires the dd takes a really long time to allocate memory with large buffers (tested with 1G) #11544 concern for this loop

The diff is net −75 lines. This run's CodSpeed result reflects it: no simulation benchmark is flagged anymore (previously default/4k_blocks/with_skip/with_seek at −3.2 to −3.8%). Local cachegrind on the codspeed analysis builds, same machine and toolchain for both sides (I refs, main 3aa5f8a vs this branch):

benchmark main this PR Δ
dd_copy_default 37,668,434 37,081,830 −1.56%
dd_copy_4k_blocks 4,741,093 4,691,701 −1.04%
dd_copy_with_skip 8,405,560 8,310,586 −1.13%
dd_copy_with_seek 8,505,965 8,408,019 −1.15%
dd_copy_partial 3,229,514 3,220,910 −0.27%
dd_copy_8k_blocks 4,409,293 4,375,979 −0.76%
dd_copy_64k_blocks 5,110,850 5,103,359 −0.15%
dd_copy_1m_blocks 9,415,242 8,422,033 −10.55%
dd_copy_separate_blocks 55,875,370 55,938,721 +0.11%

That leaves the headline number (−16.32%), which is now purely the seven Memory entries averaged: peak memory is up by exactly one page (+4,096 B, constant, not proportional to the buffer size) because the buffer over-allocates one alignment unit to place the data page-aligned. That is the fix itself — the same trade GNU dd makes via ptr_align (…, page_size) — so I'd suggest acknowledging those seven entries in CodSpeed rather than gating on them; happy to adjust if there's a preferred way to handle intentional memory deltas.

Re-verified after the refactor: cargo test -p uu_dd (90) and the dd integration suite (123) pass; outputs are byte-identical to main across an 18-configuration sweep (odd ibs/obs, swab, sync, block/unblock, count_bytes, pipes); on a dma_alignment=511 loop device, iflag=direct reads succeed and a 16-block oflag=direct copy performs 3 fcntl calls in total instead of 2 per block.

The other two failing checks look unrelated to this diff: the macOS job failed on a test_timeout signal-timing flake (all 112 dd tests pass there), and cargo-deny hit a broken action invocation this morning (unrecognized subcommand 'warn') that other PRs stopped seeing within the hour — both should clear on a re-run.

@relative23
relative23 force-pushed the dd-odirect-buffer-alignment branch from b859333 to 5868242 Compare July 17, 2026 15:43
@relative23

relative23 commented Jul 17, 2026

Copy link
Copy Markdown
Contributor Author

Round 4: the seven Memory flags are gone as well — 5868242 removes the one-page over-allocation instead of asking for it to be acknowledged.

AlignedBuffer now allocates exactly bsize bytes at the runtime page size via std::alloc::alloc_zeroed(Layout) rather than over-allocating a Vec by one alignment unit and offsetting into it. That was the source of the constant +4,096 B peak-memory delta on every dd benchmark; with the exact allocation the peak matches the plain Vec<u8> baseline byte for byte. DHAT (allocator-level peak heap, dd bs=4K over a 3 MB file):

revision peak heap vs main
main 118,055 B
previous (over-allocating) 122,117 B +4,062 B
this revision 118,021 B −34 B

Being explicit about the trade-off, since the PR previously advertised "no unsafe": this adds three small unsafe blocks (alloc_zeroed, dealloc, Deref via slice::from_raw_parts), each with a SAFETY comment. The pointer is owned by AlignedBuffer alone and freed with the same Layout it was allocated with — unlike #9104's memalign + Vec::from_raw_parts, it never crosses into an allocator-aware container, so the allocator contract holds. The unit tests pass under Miri; note that they need to be run against the module standalone (e.g. copied into a scratch crate), because the uu_dd test binary as a whole cannot run under Miri — uucore's startup code calls sigaction, which Miri does not support. I think exact allocation plus a small, auditable unsafe surface is the better end state than a permanent by-design CodSpeed exception; happy to hear if you see it differently.

Side effect on the hot loop: Deref via from_raw_parts is cheaper than the previous bounds-checked storage[offset..] reconstruction, so the simulation numbers improved again (cachegrind I refs on the codspeed analysis builds, same machine/toolchain both sides, main 058a2a6):

benchmark main this PR Δ
dd_copy_default 37,670,482 36,753,717 −2.43%
dd_copy_4k_blocks 4,743,133 4,662,232 −1.71%
dd_copy_with_skip 8,407,642 8,252,223 −1.85%
dd_copy_with_seek 8,508,047 8,348,418 −1.88%
dd_copy_8k_blocks 4,411,333 4,356,792 −1.24%
dd_copy_partial 3,231,604 3,219,789 −0.37%
dd_copy_64k_blocks 5,112,862 5,099,334 −0.26%
dd_copy_1m_blocks 9,417,324 8,424,089 −10.55%
dd_copy_separate_blocks 55,877,451 55,928,424 +0.09%

Re-verified after the change: 91 unit + 123 integration tests pass, outputs byte-identical to main across the 18-configuration sweep, and on a dma_alignment=511 loop device iflag=direct reads succeed while a 16-block oflag=direct copy performs 3 fcntl calls in total instead of 2 per block. The branch is also rebased onto current main (058a2a6).

@relative23

Copy link
Copy Markdown
Contributor Author

Hardened the buffer internals a bit further ahead of review: AlignedBuffer now stores the Layout it validated at construction, so the deallocation invariant is simply "freed with exactly the stored layout" (and the unwrap is gone from the Drop path). The alignment contract is now enforced for zero-length buffers as well — previously try_new(0, 3) succeeded in release builds — and empty buffers get an aligned dangling pointer, so the alignment promise holds for them too. New edge-case tests cover rejected alignments and an unrepresentable size; the module's six unit tests still pass under Miri (standalone, as noted above).

One correction to my earlier phrasing for precision: the unsafe surface is four unsafe expressions in three categories — allocation, deallocation, and the two Deref slice constructions. No behavioral change to the copy loop; the DHAT peak is unchanged.

Comment thread src/uu/dd/src/aligned_buffer.rs Outdated
Comment thread src/uu/dd/src/aligned_buffer.rs Outdated
Comment thread src/uu/dd/src/aligned_buffer.rs Outdated
Comment thread src/uu/dd/src/aligned_buffer.rs Outdated
Comment thread src/uu/dd/src/dd.rs Outdated
Comment thread src/uu/dd/src/dd.rs Outdated
Comment thread tests/by-util/test_dd.rs Outdated
Comment thread tests/by-util/test_dd.rs Outdated
Comment thread tests/by-util/test_dd.rs Outdated
@sylvestre

sylvestre commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

i stopped reviewing it because i became tired ;)
I do appreciate the improvements that you are making but you need to use your AI better

A few comments:

  • it is clearly written by an AI (i don't mind) but I mind that you aren't using it to make my life easier and these PR are impossible to review
  • all your comments in this PR are too long
  • This PR is WAY too big
  • Too many unsafe
  • please leverage other crates that we already use
  • Don't follow our practices/recommendations
  • If you improved the performances, write divan's benchmark to demonstrate this, i don't care about comments as they don't scale

I will stop there too but maybe i should start providing an AGENTS.md and skills to help contributors

@relative23

Copy link
Copy Markdown
Contributor Author

Thanks, understood. Before reworking this: should I keep #13373 to the minimal alignment fix and move the probe/strace tests to a follow-up? rustix::mm::mmap is also unsafe; I can use safe memmap2::MmapMut::map_anon (already in the workspace) or the safe over-allocated Vec. Which do you prefer? I will drop the performance prose unless it is backed by divan.

@relative23
relative23 force-pushed the dd-odirect-buffer-alignment branch from 8624f83 to 42f8ea2 Compare July 21, 2026 20:06
@relative23
relative23 force-pushed the dd-odirect-buffer-alignment branch from 42f8ea2 to 95d9fe9 Compare July 21, 2026 21:36
@relative23 relative23 changed the title dd: align the I/O buffer to the page size for O_DIRECT dd: page-align the read buffer for iflag=direct Jul 22, 2026
@relative23

Copy link
Copy Markdown
Contributor Author

Reworked this around a safe, Vec-backed buffer; the PR now adds no unsafe code. I removed the probe/strace scaffolding, leaving the read-buffer fix and focused regression tests. CodSpeed reports no performance change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

dd iflag=direct reads fail with "IO error: Invalid input" on devices with strict dma_alignment

3 participants